http: incoming parser set to null twice causes crash #4948
Conversation
|
it looks like freeParser already sets it to null Would just checking if it's already been freed/set to null be ok? |
|
By the way this is how to reproduce: var http = require('http');
http.createServer(function(serverReq, serverRes){
http.request({hostname: 'google.com', port: 80}, function(res) {
res.on('data', function(data) { }); // this is required to reproduce
serverRes.end(); // this is required this to reproduce
}).end();
}).listen(8080);Send a request to the server listening on 8080 and close the connection early. The two required pieces of line to reproduce I'm guessing one is calling freeParser() and the other is calling _dump(). So |
|
Can you add a regression test in test/simple or test/internet? (One that preferably doesn't need external services.) Also, please see https://github.com/joyent/node/blob/master/CONTRIBUTING.md - esp. the bits about the CLA and the commit log. |
Fix nodejs#4948 This adds a check before setting the incoming parser to null. Under certain circumstances it'll already be set to null by freeParser(). Otherwise this will cause node to crash as it tries to set null on something that is already null.
| res.on('data', function(data) {}); | ||
| }).end(); | ||
|
|
||
| }).listen(8080); |
|
No it doesn't need to contact an external site, it can make a request to the created server. I just thought it would help clarify use case.
|
|
Thanks! Landed on 9f4c3b0. |
|
Aaaannnd reverted on 632b7d8. My bad. This test doesn't pass with the commit, and also makes a bunch of other tests fail. |
| var common = require('../common'); | ||
| var http = require('http'); | ||
|
|
||
| http.createServer(function(serverReq, serverRes){ |
There was a problem hiding this comment.
This server listens, but never calls server.close() so the test script will stay open forever and time out.
|
Sorry for the oversight |
this test crashes as it'll try to set the incoming parser to null twice
Fix #4948 This adds a check before setting the incoming parser to null. Under certain circumstances it'll already be set to null by freeParser(). Otherwise this will cause node to crash as it tries to set null on something that is already null.
|
Landed in v0.10 on 5757ce4. Thanks! |
The error: